Conversation
a5a3acf to
967f281
Compare
There was a problem hiding this comment.
Pull request overview
Implements POSIX/WASI-like open_at behavior in the in-memory VFS so guests can open existing nodes and create/truncate files (a prerequisite for enabling host-side FS writes), and updates unit/integration tests to reflect the new semantics.
Changes:
- Refactors VFS path resolution and expands
VfsCtxView::open_atto handleCREATE,EXCLUSIVE,DIRECTORY, andTRUNCATE. - Adds extensive unit tests around
open_atbehavior and updates integration test expectations/snapshots. - Makes
Limiter::growtake&self(internally synchronized), simplifying ownership/mutability in component setup.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| host/src/vfs/mod.rs | Implements new open_at semantics, factors path traversal helper, and adds unit tests. |
| host/src/limiter.rs | Changes grow to &self to allow shared use behind a mutex. |
| host/src/component.rs | Adjusts limiter initialization to match the new Limiter::grow signature/usage. |
| host/tests/integration_tests/python/runtime/fs.rs | Updates Python FS integration test to exercise create-on-open behavior. |
| host/tests/integration_tests/evil/fs.rs | Updates large snapshot expectations for new VFS/open behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d7bd5c4 to
adb81f9
Compare
| let limiter = Limiter::new(self.static_limits, &pool); | ||
| let vfs_state = VfsState::new(limits, limiter); | ||
| let table = ResourceTable::new(); | ||
| (table, vfs_state) |
There was a problem hiding this comment.
We return a non-mutable tuple here instead of the VfsCtxView directly, as wrapping these in the VfsCtxView requires that they be mutable references, and rustc doesn't like that 😞
6278d31 to
9d61c8c
Compare
| | | . | ERR: Is a directory (os error 31) | | ||
| | | .. | ERR: Is a directory (os error 31) | | ||
| | | / | ERR: Is a directory (os error 31) | | ||
| | | /bin | ERR: Bad file descriptor (os error 8) | |
There was a problem hiding this comment.
Shouldn't this result in "invalid" or "not found" or something? "Bad file descriptor" kinda sound like an internal mess-up.
There was a problem hiding this comment.
There's really only two places we return BadDescriptor. I'm still trying to nail down why exactly this is happening.
There was a problem hiding this comment.
Yeah we're hitting this, so the resource we're trying to get doesn't exist in the resource table.
This kind of makes sense though doesn't it? if none of the source/destinations exist; since we start with a new VFS for each test.
There was a problem hiding this comment.
The guest code however doesn't create Resource<Descriptor> out of thin air (I think, at least if it's not buggy). So we must have handed out a descriptor that is broken or something? Or do we have some kind of type confusion or broken cast at some point?
There was a problem hiding this comment.
Maybe a broken cast? I have no idea what could be the cause. I think if something is going wrong it's related to get_directories - as that's where the resource is initially created and added the ResourceTable. Setting breakpoints here and here, and then running test_copy, we first push a descriptor on to the table, and then when we get it we get BadDescriptor - which is only returned if the the descriptor isn't found in the resource table - so something is not quite adding up.
What's strange is examining the ResourceTable when hitting the BadDescriptor breakpoint shows that it contains an entry; and that entry points to the same value as what's added to it in get_directories.
Reading more about std::fs::copy(from, to), there is this tidbit regarding an error case:
fromis neither a regular file nor a symlink to a regular file.
To me this reads that if the file it does try to copy is a directory, it will fail since a directory is neither of those things. And since ., .., and / all exist on the VFS but are directories. If I run this test locally, I do encounter the .. neither a regular file nor a symlink .. error. So that also points to this being the cause of the error.
As for what we should do about this - maybe just remove those values specifically from PATHS? I don't really see that as ideal either, as we definitely want to test those.
I wonder if maybe we should break up the evil tests by files that we expect to be in the VFS vs. files we don't expect to be there?
EG, we could have another constant EXISTING_VFS_PATHS that consists of that list (., .., /) and combine it with PATHS for most tests, but then break it up for certain tests like test_copy. Just kind of a spitball idea ATM though.
862b2a5 to
259bdb4
Compare
…et_node_from_start
71e926e to
c750feb
Compare
Closes #336
I followed the specification described here. Given that we will eventually need to test this against the wasi test-suite, any differences between the aforementioned spec and the test-suite will favor the test-suite; so significant changes may need to be made in the future.
Describe your proposed changes here.